Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(breadcrumbs): improved rendering of breadcrumb progress bar #44450

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

emoral435
Copy link
Contributor

@emoral435 emoral435 commented Mar 25, 2024

Summary

See #44162 (comment) for context on this PR.
Essentially, users currently cannot browse and navigate to other files while an upload is in progress, so this change fixes the logic dedicated to rendering the breadcrumbs.
We now only hide breadcrumbs when the width of the file list is 400px AND when there is an upload in progress.

firefox_bkEM8LLGhh.mp4

Checklist

@emoral435 emoral435 force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 7774bfc to a4cae4e Compare March 25, 2024 11:35
@emoral435 emoral435 requested a review from susnux March 25, 2024 11:36
@emoral435 emoral435 self-assigned this Mar 25, 2024
@susnux
Copy link
Contributor

susnux commented Mar 25, 2024

Should we not always show the breadcrumbs? Otherwise mobile users can not navigate, no?
And for mobile let the uploader progress wrap (second line below breadcrumbs).

Maybe instead of breadcrumbs show a select component with the paths as option on small mobile?

@emoral435 emoral435 changed the title fix(breadcrumbs): improved conditional rendering logic of breadcrumbs fix(breadcrumbs): improved rendering of breadcrumb progress bar Mar 25, 2024
@emoral435 emoral435 force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from a4cae4e to 381363f Compare March 25, 2024 20:11
@emoral435
Copy link
Contributor Author

@susnux I changed the title and description of the PR to match the changes you suggested now! On mobile / small screens, we just wrap the progress bar on a newline now :) Reasoning for the 512px is because we use this size here :

<NcButton v-if="canShare && filesListWidth >= 512"

@emoral435 emoral435 requested a review from susnux March 26, 2024 12:36
Copy link
Contributor

@susnux susnux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works!

But this makes a bug of the breadcrumbs more visible: They overflow their container sometimes. This also happens on master but is very visible now.
(But needs to be fixed on the components part)

@emoral435 emoral435 requested a review from nfebe March 26, 2024 13:32
@emoral435
Copy link
Contributor Author

Yup, I agree @susnux . Just testing it out, and the overflow can get quite clunky here and there. The solution you proposed on the design team is good to me!

firefox_SWtd1aJUhD.mp4

Here is a video in case you need to showcase what you mean to designers.

@susnux
Copy link
Contributor

susnux commented Mar 26, 2024

What is missing is making the uploader "new" button icon only on <370px width. But this needs to be fixed on the library I think.

@skjnldsv
Copy link
Member

Considering the changes and the discussion here, please loop the design team in before any approval or merge :)

@susnux
Copy link
Contributor

susnux commented Mar 26, 2024

Considering the changes and the discussion here, please loop the design team in before any approval or merge :)

Already done in design chat

@susnux
Copy link
Contributor

susnux commented Mar 27, 2024

CC @nextcloud/designers what do you think? The very small breakpoint (icon only new-button) is not implemented as this needs fixes on the library.

Full bread crumbs with share button full bread crumbs without share button Icon only first breadcrumb Smallest possible bread crumbs with full new-button
Screenshot_20240327_130623 Screenshot_20240327_130639 Screenshot_20240327_130708 Screenshot_20240327_130722

And this would be a mock of our smallest supported width of 300px, here I made the new-button icon only and reduced the min-width of the last breadcrumb to 72px so it does not overlap with the button:
image

@susnux susnux force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch 2 times, most recently from 62f91fd to 3a92ff3 Compare March 27, 2024 12:39
Copy link
Member

@jancborchardt jancborchardt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And this would be a mock of our smallest supported width of 300px, here I made the new-button icon only and reduced the min-width of the last breadcrumb to 72px so it does not overlap with the button

Looks good @susnux – I would even say that "All files" can get moved into the 3-dot menu before the "New" needs to get cut from the button label, as it saves a bunch more space.

@emoral435
Copy link
Contributor Author

@jancborchardt That can definitely be done! Just needs to be changed here within this logic, AFAIK :) :

https://github.com/nextcloud-libraries/nextcloud-vue/blob/d11028854b8cceba83725e417eb7b3563bcc2832/src/components/NcBreadcrumbs/NcBreadcrumbs.vue#L291-L299

However, just as a counterpoint, the last breadcrumb already just shows the user the directory they are in. Clicking on it just would take them to the same place - so isn't it more useful to have all files stay? Plus, we would be able to keep the cool icon :)

I do not have a huge opinion on this, so whatever you decide is best :)

@emoral435
Copy link
Contributor Author

So AFAIK, the changes that are needed for the breadcrumb are all library-dep changes.... so for the time being, anyone want to give this PR a green check 👀💚

@jancborchardt
Copy link
Member

jancborchardt commented Apr 2, 2024

However, just as a counterpoint, the last breadcrumb already just shows the user the directory they are in. Clicking on it just would take them to the same place - so isn't it more useful to have all files stay?

It not only is a link though, it also is the only place where we show the name of the folder you are in. And that is more important to know than to quickly go back to "All files". :) (Good for asking though!)

@susnux
Copy link
Contributor

susnux commented Apr 2, 2024

So I would say we start with this one and the either move the "all files" into the overflow menu (nextcloud-vue changes) or make the "new +" button icon only (nextcloud-upload change) later?

@emoral435
Copy link
Contributor Author

So I would say we start with this one and the either move the "all files" into the overflow menu (nextcloud-vue changes) or make the "new +" button icon only (nextcloud-upload change) later?

I agree! This PR gets the mobile view to have correct reactivity, then we can improve it even further by what you mentioned. I will tackle the nextcloud-vue library, do you want to tackle the upload-library?

@skjnldsv skjnldsv force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 3a92ff3 to fb8fdd8 Compare April 4, 2024 16:53
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 4, 2024
@skjnldsv skjnldsv force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from fb8fdd8 to 96e798f Compare April 4, 2024 16:55
@skjnldsv skjnldsv enabled auto-merge April 4, 2024 17:04
@emoral435 emoral435 force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 96e798f to 889105a Compare April 5, 2024 00:45
@skjnldsv skjnldsv added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels Apr 5, 2024
@skjnldsv
Copy link
Member

skjnldsv commented Apr 5, 2024

Related cypress

  1) Files: Live photos
       'Show hidden files' is enabled
         Restores files when restoring the .jpg:
     AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-cy-files-content-breadcrumbs]`, but never found it.
      at n (webpack://nextcloud/./cypress/e2e/files/FilesUtils.ts:119:7)
      at Context.eval (webpack://nextcloud/./cypress/e2e/files/live_photos.cy.ts:147:12)

  2) Files: Live photos
       'Show hidden files' is enabled
         Blocks restoration when restoring the .mov:
     AssertionError: Timed out retrying after 4000ms: Expected to find element: `[data-cy-files-content-breadcrumbs]`, but never found it.
      at n (webpack://nextcloud/./cypress/e2e/files/FilesUtils.ts:119:7)
      at Context.eval (webpack://nextcloud/./cypress/e2e/files/live_photos.cy.ts:158:12)

@emoral435
Copy link
Contributor Author

emoral435 commented Apr 8, 2024

Weird, we did not change anything that should be breaking the cypress 🙈 Will investigate and re-run to see if what works!

EDIT: Trying a rebase?? Cypress seems weird rn...

@emoral435 emoral435 force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 889105a to 955bd3e Compare April 8, 2024 17:43
@susnux
Copy link
Contributor

susnux commented Apr 9, 2024

@emoral435

) Files: Live photos
'Show hidden files' is enabled
Restores files when restoring the .jpg:
AssertionError: Timed out retrying after 4000ms: Expected to find element: [data-cy-files-content-breadcrumbs], but never found it.
at n (webpack://nextcloud/./cypress/e2e/files/FilesUtils.ts:119:7)
at Context.eval (webpack://nextcloud/./cypress/e2e/files/live_photos.cy.ts:147:12)

Cypress seems to be related

@emoral435 emoral435 force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 955bd3e to 7ba8e8e Compare April 9, 2024 13:44
@susnux susnux force-pushed the emoral435/fix/fix-breadcrumb-showing-capabilities branch from 7ba8e8e to c252ee5 Compare April 9, 2024 23:20
@skjnldsv skjnldsv merged commit cc81480 into master Apr 9, 2024
104 checks passed
@skjnldsv skjnldsv deleted the emoral435/fix/fix-breadcrumb-showing-capabilities branch April 9, 2024 23:39
Copy link

backportbot bot commented Apr 9, 2024

The backport to stable29 failed. Please do this backport manually.

# Switch to the target branch and update it
git checkout stable29
git pull origin stable29

# Create the new backport branch
git checkout -b backport/44450/stable29

# Cherry pick the change from the commit sha1 of the change against the default branch
# This might cause conflicts, resolve them
git cherry-pick de47a9ef f6b1fd41 0213fb6b c252ee55

# Push the cherry pick commit to the remote repository and open a pull request
git push origin backport/44450/stable29

Error: Failed to push branch backport/44450/stable29: fatal: could not read Username for 'https://github.com': No such device or address


Learn more about backports at https://docs.nextcloud.com/server/stable/go.php?to=developer-backports.

@susnux
Copy link
Contributor

susnux commented Apr 9, 2024

/backport to stable29

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants